Could we add a comment somewhere here explaining that if the graph import_path or runtime_env are changed, Serve considers that a code change?

Checking this pr, it makes me feel the _generate_new_version_config function name is confusing. @shrekris-anyscale @zcin I thought it is always generate a new version of config... can we try to rename the function? And I feel we can hide these logics into the function _generate_new_version_config directly. WDYT?

That's a good point– _generate_new_version_config makes it sound like we're creating a new version for the whole config. In reality, it generates new versions for each of the deployments.

I think we should rename the function to something like _generate_deployment_config_versions and keep the functionality as is, or we should rename it to _version_config and take in two configs. Since the next release is soon, we should probably pick whichever one gives us more confidence in being well-tested. @sihanwang41 @zcin what do you think?

yeah, let use _generate_deployment_config_versions first.

I feel it worth refactoring remove the logics inside the deploy_app level, it is a little burden in deploy_app level to "know" when should i pass config_dict, last_config_dict, last_version_dict when should I pass config_dict, {}, {} since the _generate_new_version_config is to determines whether each deployment's version should be changed based on the updated options (from the function description). deploy_app doesn't need to know the trick here :)

But if too much for 2.0 release, we can figure out later.

The current functionality also takes in two configs, right?
Perhaps we can rename to _update_deployment_config_versions?

@sihanwang41 Do you mean to hide all the logic, including the loading from kv_store, or just the new logic that checks import_path and runtime_env?

_generate_new_version_config's current functionality takes in two dictionaries containing only the deployment configs. It doesn't take in two full configs that match the ServeApplicationSchema format (including the top-level import_path and runtime_env).

I think @sihanwang41 was saying we could pass in entire configs and do the top-level import_path and runtime_env handling in _generate_new_version_config. That way, we wouldn't have to pass in just the deployment options into the helper function.

the goal is to hide burden for deploy_app to know whether i should pass last_config_dict or {}. we can keep the interface, but the logic in this pr can be hidden.

When checkpoint is None, we can directly pass _generate_new_version_config(config_dict) and set last_deployed_config and last_deployed_versions default value.

Actually, I think currently _generate_new_version_config takes in full configs that match the ServeApplicationSchema format (albeit in dictionary form). Hiding the logic into the function _generate_new_version_config makes a lot of sense, though! I think I can make that change easily.